Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

GH-2852: Provide a way to customize a transactionIdSuffix #2930

Merged
merged 2 commits into from
Jan 19, 2024
Merged

GH-2852: Provide a way to customize a transactionIdSuffix #2930

merged 2 commits into from
Jan 19, 2024

Conversation

stillya
Copy link
Contributor

@stillya stillya commented Dec 10, 2023

Introduce a new TransactionSuffixManager interface with DefaultTransactionSuffixManager implementation to restrict transaction.id in range for existing PR

Resolves #2852

This was referenced Dec 10, 2023
Closed
Closed
@stillya stillya changed the title GH-2851: Provide a way to customize a transactionIdSuffix GH-2852: Provide a way to customize a transactionIdSuffix Dec 10, 2023
@stillya stillya force-pushed the gh-2852/tx-generator branch from e79d724 to 4e8237d Compare December 10, 2023 20:18
@sobychacko
Copy link
Contributor

@stillya @Wzy19930507 It looks like we can use this PR instead of the previous one since this contains all the commits from the previous one (except for the docs commit). If that is the case, can we close the previous PR?

@sobychacko
Copy link
Contributor

Checkstyle failures in the PR build: https://github.com/spring-projects/spring-kafka/actions/runs/7160082920

@sobychacko sobychacko self-assigned this Dec 11, 2023
@sobychacko
Copy link
Contributor

sobychacko commented Dec 12, 2023 •
edited
Loading

@stillya The gist of your change from a first-pass review I did is the following.

The DefaultKafkaProducerFactory is now aware of a TransactionSuffixManager. with an implementation of DefaultTransactionSuffixManager. The contract for the TransactionSuffixManager allows three things - retrieve, return, and reset transaction suffixes. When the transactional producer is created for the first time, the suffix manager primes the suffix cache with the maxCache - 1 suffixes, which the producer factory retrieves to create the transactional producer. The suffix retrieval will continue if more producers are created via the factory. Once the suffix cache is full, If a request comes in for creating another transactional producer, then the NoProducerAvailableException is thrown. The suffix is returned to the cache when the producers are closed, or transaction exceptions occur, etc. When the producer factory is destroyed, then the suffix cache is reset. I think this is the main use case that we are addressing via this PR. Could you please confirm?

Some quick comments. The TransactionSuffixManager might sound a bit heavy in this context, as we expose this as a public API. I suggest using a lighter API name for both the interface and the implementation.

When maxCache is zero, where is the default code that increments the suffix re-located?

Thanks!

@Wzy19930507
Copy link
Contributor

Wzy19930507 commented Dec 12, 2023 •
edited
Loading

It looks like we can use this PR instead of the previous one since this contains all the commits from the previous one (except for the docs commit). If that is the case, can we close the previous PR?

@sobychacko Sure, let’s continue in this PR, @stillya I’m sorry misunderstood your meaning.

@stillya
Copy link
Contributor Author

stillya commented Dec 12, 2023

@stillya The gist of your change from a first-pass review I did is the following.

The DefaultKafkaProducerFactory is now aware of a TransactionSuffixManager. with an implementation of DefaultTransactionSuffixManager. The contract for the TransactionSuffixManager allows three things - retrieve, return, and reset transaction suffixes. When the transactional producer is created for the first time, the suffix manager primes the suffix cache with the maxCache - 1 suffixes, which the producer factory retrieves to create the transactional producer. The suffix retrieval will continue if more producers are created via the factory. Once the suffix cache is full, If a request comes in for creating another transactional producer, then the NoProducerAvailableException is thrown. The suffix is returned to the cache when the producers are closed, or transaction exceptions occur, etc. When the producer factory is destroyed, then the suffix cache is reset. I think this is the main use case that we are addressing via this PR. Could you please confirm?

Yes, you're absolutely right.

Some quick comments. The TransactionSuffixManager might sound a bit heavy in this context, as we expose this as a public API. I suggest using a lighter API name for both the interface and the implementation.

I agree that TransactionSuffixManager might sound a bit heavy for this context. Do you have any suggestions for a lighter, more intuitive name for both the interface and its implementation? Maybe TransactionSuffixHandler as an alternative?

When maxCache is zero, where is the default code that increments the suffix re-located?

When maxCache is zero, so caching is disabled, then we go here and just increment the counter as before.

@sobychacko
Copy link
Contributor

sobychacko commented Dec 12, 2023 •
edited
Loading

I agree that TransactionSuffixManager might sound a bit heavy for this context. Do you have any suggestions for a lighter, more intuitive name for both the interface and its implementation? Maybe TransactionSuffixHandler as an alternative?

Handler sounds reasonable. How about TransactionIdSuffixHandler and DefaultTransactionIdSuffixHandler?

@sobychacko
Copy link
Contributor

sobychacko commented Dec 12, 2023 •
edited
Loading

@stillya, The PR CI build keeps failing due to some test failure. Could you take a look? https://github.com/spring-projects/spring-kafka/actions/runs/7186689052

@stillya
Copy link
Contributor Author

stillya commented Dec 12, 2023

@stillya, The PR CI build keeps failing due to some test failure. Could you take a look? https://github.com/spring-projects/spring-kafka/actions/runs/7186689052

Yeah, sorry, broke one test by reverting old code. Fixed, now everything should pass.

@sobychacko sobychacko self-requested a review December 12, 2023 21:34
artembilan
artembilan requested changes Dec 12, 2023
View reviewed changes
Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, find some review from me.

In general looks good, but there are a couple simple concerns yet.

Thanks

spring-kafka-docs/src/main/antora/modules/ROOT/pages/kafka/transactions.adoc Outdated Show resolved Hide resolved
spring-kafka-docs/src/main/antora/modules/ROOT/pages/kafka/transactions.adoc Outdated Show resolved Hide resolved
spring-kafka/src/main/java/org/springframework/kafka/core/DefaultKafkaProducerFactory.java Show resolved Hide resolved
...g-kafka/src/main/java/org/springframework/kafka/core/DefaultTransactionIdSuffixStrategy.java Outdated Show resolved Hide resolved
spring-kafka/src/main/java/org/springframework/kafka/core/NoProducerAvailableException.java Outdated Show resolved Hide resolved
spring-kafka/src/main/java/org/springframework/kafka/core/NoProducerAvailableException.java Outdated Show resolved Hide resolved
spring-kafka/src/main/java/org/springframework/kafka/core/TransactionIdSuffixStrategy.java Outdated Show resolved Hide resolved
spring-kafka/src/main/java/org/springframework/kafka/core/TransactionIdSuffixStrategy.java Outdated Show resolved Hide resolved
@sobychacko sobychacko added this to the 3.2.0-M1 milestone Dec 13, 2023
@artembilan
Copy link
Member

FYI, we have pushed this change to the next 3.2 generation as a new feature request.
Too drastic to make a change in the current patch release.

Thanks

@sobychacko sobychacko removed this from the 3.2.0-M1 milestone Dec 13, 2023
@stillya stillya requested a review from artembilan December 20, 2023 19:17
@artembilan
Copy link
Member

@stillya ,

please, rebase your branch to the latest main.

Then we will start looking into your change to incorporate it into a new version.

Thanks

@stillya stillya force-pushed the gh-2852/tx-generator branch from 0bf30c2 to ccedad6 Compare January 18, 2024 19:47
@stillya stillya force-pushed the gh-2852/tx-generator branch from ccedad6 to 5e71cf8 Compare January 18, 2024 19:53
artembilan
artembilan requested changes Jan 18, 2024
View reviewed changes
Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, consider to update copyright of all the affected classes to the current year.

...g-kafka/src/main/java/org/springframework/kafka/core/DefaultTransactionIdSuffixStrategy.java Outdated Show resolved Hide resolved
spring-kafka/src/main/java/org/springframework/kafka/core/DefaultKafkaProducerFactory.java Show resolved Hide resolved
@stillya stillya force-pushed the gh-2852/tx-generator branch from 1e403d4 to 8277995 Compare January 18, 2024 20:52
artembilan
artembilan approved these changes Jan 19, 2024
View reviewed changes
Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, I'm OK with this.

@sobychacko , you call.

Thanks

@sobychacko
Copy link
Contributor

Thanks, Artem! I will take a look and merge it today.

@sobychacko sobychacko merged commit b9c9abd into spring-projects:main Jan 19, 2024
3 checks passed
@sobychacko
Copy link
Contributor

@stillya @Wzy19930507 Thank you for this significant PR contribution. It is now merged upstream.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Wzy19930507 Wzy19930507 Wzy19930507 requested changes

@NathanQingyangXu NathanQingyangXu NathanQingyangXu left review comments

@artembilan artembilan artembilan approved these changes

@sobychacko sobychacko sobychacko approved these changes

Assignees

@sobychacko sobychacko

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Provide a way to customize a transactionIdSuffix
5 participants
@stillya @sobychacko @Wzy19930507 @artembilan @NathanQingyangXu